-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[main] Introduce cmdline option parser to share some code between executables #20073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Test Results 20 files 20 suites 3d 6h 38m 21s ⏱️ For more details on these failures, see this check. Results for commit 16da20f. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (Beside the comments and portability issue)
|
Thanks a lot for the initiative! Even if it's 300 lines of code, isn't it better to rely on something external that gets auto-maintained, even if it's bigger in number of lines of code? Or to understand better the motivation, what do we gain by having a small parser for which we have to add our own tests?
|
It's mostly the last point (though the first is also somewhat important). |
d475a82 to
3e4fe8b
Compare
|
On the flipside, a library that is heavily used is more likely to come across all of the weird edge cases that are going to end up making the custom implementation significantly longer eventually. When I designed CLI11, I did have ROOT in mind, and with that you could actually make it easy to have a user extend an application. That would definitely be more than 10x functionality. |
This is an unprovable assumption as of now - if we start finding such edge cases regularly then this might tilt the decision towards an external library. It's very easy to change our mind later after all, this is all "leaf code", which brings me to the second point:
I don't see the point here: the user cannot "extend" a binary like
Again, this is conjecture which can only be proven with actually trying both things. My position is: let's start with the simpler option, then if we realize it was a mistake we can easily swap the implementation later. |
e709b30 to
9ef69b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Two general comments:
- I'd call "boolean flag" a switch
- Perhaps call out in the initial documentation that repeated flags are not supported (e.g., no
root-ssh -vvv)
|
|
||
| struct RFlag { | ||
| std::string fName; | ||
| std::string fValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an optional to distinguish a switch from an empty string argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the distinction relevant? As far as I see it, it would only be relevant if the user is iterating the flags list and wants to know which flag is a switch and which is not; but the user knows the semantic value of the flags already (as he defined them), so my impression is that it's fine to collapse the two cases.
But maybe I'm missing some other benefits to this, feel free to let me know if you have any in mind
| std::string_view help = "") | ||
| { | ||
| int aliasIdx = -1; | ||
| for (auto f : aliases) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: check that the alias is neither - nor contains a space (and possibly other funny stuff, newline, NULL, etc). Maybe enough to say in the documentation what is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alias already cannot be - (or start with a - after the prefix) as that is prevented by the combination of the two ifs; I could validate the characters, but I think it's a bit overly defensive (the user will almost always define these aliases as literal strings and probably won't shoot themselves in the foot by using weird characters in flags)
c8b6aea to
ca1bf2d
Compare
…on Mac See comment in the code as to why
We now have enough C++ executables (with more to be ported) to justify sharing some code between them, in particular option parsing.
This PR introduces a simple option parser class that covers most of our cases. Not all cases (e.g.
haddkeeps its custom parsing because it's a bit weird), but it's already enough to coverrootlsandrootbrowse, and will in the future also coverrootcp,rootmvetc.The parser is documented with code examples and properly tested and it's about 300 lines of codes (about an order of magnitude less than e.g. cxxopts which in my opinion is overkill for our use case).
Checklist: